Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Exclude pgp signtures as attachments #5602

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Exclude pgp signtures as attachments #5602

merged 1 commit into from
Dec 4, 2023

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Oct 1, 2021 •
edited
Loading

Story: I tried to forward an email with an OpenPGP signature.

image

This led to an internal server error: Argument 2 passed to OCA\\Mail\\Model\\Message::addRawAttachment() must be of the type string, null given, called in mail/lib/Service/MailTransmission.php on line 412

1: Show the email body and attachments

Part.php:1457, Horde_Mime_Part->isAttachment()
IMAPMessage.php:396, OCA\Mail\Model\IMAPMessage->getPart()
IMAPMessage.php:375, OCA\Mail\Model\IMAPMessage->loadMessageBodies()
IMAPMessage.php:100, OCA\Mail\Model\IMAPMessage->__construct()
MessageMapper.php:219, OCA\Mail\IMAP\MessageMapper->OCA\Mail\IMAP\{closure:/var/www/html/apps-extra/mail/lib/IMAP/MessageMapper.php:216-233}()
MessageMapper.php:233, array_map()
MessageMapper.php:233, OCA\Mail\IMAP\MessageMapper->findByIds()
MessageMapper.php:65, OCA\Mail\IMAP\MessageMapper->find()
MailManager.php:180, OCA\Mail\Service\MailManager->getImapMessage()
MessagesController.php:249, OCA\Mail\Controller\MessagesController->getBody()
Dispatcher.php:217, OC\AppFramework\Http\Dispatcher->executeController()
Dispatcher.php:126, OC\AppFramework\Http\Dispatcher->dispatch()
App.php:157, OC\AppFramework\App::main()
Router.php:302, OC\Route\Router->match()
base.php:1000, OC::handleRequest()
index.php:36, {main}()

2: Forward an email with openpgp attachment

Part.php:1457, Horde_Mime_Part->isAttachment()
MessageMapper.php:501, OCA\Mail\IMAP\MessageMapper->getRawAttachments()
MailTransmission.php:457, OCA\Mail\Service\MailTransmission->handleForwardedAttachment()
MailTransmission.php:356, OCA\Mail\Service\MailTransmission->handleAttachments()
MailTransmission.php:158, OCA\Mail\Service\MailTransmission->sendMessage()
AccountsController.php:433, OCA\Mail\Controller\AccountsController->send()
Dispatcher.php:217, OC\AppFramework\Http\Dispatcher->executeController()
Dispatcher.php:126, OC\AppFramework\Http\Dispatcher->dispatch()
App.php:157, OC\AppFramework\App::main()
Router.php:302, OC\Route\Router->match()
base.php:1000, OC::handleRequest()
index.php:36, {main}()

loadMessageBodies is using $structure->getParts() to fetch the parts of a message.

image

getRawAttachments is using $structure->partIterator() to fetch the parts of a message.

image

The object can now be iterated through to access the subparts. partIterator() can be used to obtain a RecursiveIteratorIterator that will iterator through all descendants. The $parent property has been added; it will be set, and is only guaranteed to be accurate, during iteration.

When using partIterator the $parent property is set. getParts does not set the $parent property.

Horde_Mime_Part.isAttachment is using the $parent property to detect if an pgp-signature is an attachment or not: https://github.com/horde/Mime/blob/7f6d0aaea1a1f194f46ac4a0428b539df11b46e1/lib/Horde/Mime/Part.php#L1464-L1482

tl;dr When viewing the message we see an attachment but when forwarding the same message we don't find the attachment anymore because isAttachment return different result.

I suspect that changing $structure->getParts() to $structure->partIterator() in loadMessageBodies is the proper fix. Yet that seems like a bigger change.

@kesselb kesselb self-assigned this Oct 4, 2021
@kesselb kesselb mentioned this pull request May 12, 2022
Closed
@ChristophWurst
Copy link
Member

rebase, resolve and merge :shipit:

@kesselb kesselb force-pushed the bug/noid/exclude-pgp-and-other-keys branch from 3d4ea65 to 56c66be Compare June 28, 2023 13:10
@kesselb
Copy link
Contributor Author

kesselb commented Jun 28, 2023

Code moved from IMAPMessage to ImapMessageFetcher.

@ChristophWurst ChristophWurst marked this pull request as draft June 28, 2023 13:14
@kesselb kesselb force-pushed the bug/noid/exclude-pgp-and-other-keys branch from 56c66be to 4ed234a Compare July 4, 2023 14:36
ChristophWurst
ChristophWurst approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks scary but good

@kesselb
Exclude pgp signtures as attachments
14bc67e 
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb force-pushed the bug/noid/exclude-pgp-and-other-keys branch from 4ed234a to 14bc67e Compare September 30, 2023 18:06
@kesselb
Copy link
Contributor Author

kesselb commented Sep 30, 2023 •
edited
Loading

Thanks for the reminder 👍

Rebased the branch.

It should be much easier to review now because #8597 was merged a while ago.

I will take a look next week and mark it as ready if we still need it.

@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Oct 2, 2023
Open
@ChristophWurst ChristophWurst mentioned this pull request Nov 29, 2023
Closed
24 tasks
@ChristophWurst ChristophWurst marked this pull request as ready for review December 4, 2023 11:32
@ChristophWurst ChristophWurst requested a review from st3iny December 4, 2023 11:32
st3iny
st3iny approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works.

Christoph's advent of code is going strong 💪 🎄 🎁

@st3iny st3iny merged commit 34b4a71 into main Dec 4, 2023
29 checks passed
@st3iny st3iny deleted the bug/noid/exclude-pgp-and-other-keys branch December 4, 2023 12:06
@st3iny
Copy link
Member

st3iny commented Dec 4, 2023

/backport to stable3.5

@st3iny st3iny added this to the v3.6.0 milestone Dec 4, 2023
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Dec 4, 2023
Merged
@ChristophWurst
Copy link
Member

Christoph's advent of code is going strong 💪 🎄 🎁

Sadly this is the first PR I could really get merge 🫠

@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Dec 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st3iny st3iny st3iny approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@StCyr StCyr Awaiting requested review from StCyr

Assignees

@kesselb kesselb

Labels
3. to review bug
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@kesselb @ChristophWurst @st3iny